-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade to React 18 #1100
Upgrade to React 18 #1100
Conversation
Regarding |
As noted elsewhere,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the code changes, and it looks really great. I made some nitpicky suggestions, mostly related to keeping things as type-safe as possible. Let me know what you think!
packages/libs/eda/src/lib/core/components/filter/HistogramFilter.tsx
Outdated
Show resolved
Hide resolved
packages/libs/wdk-client/src/Components/Display/CollapsibleSection.tsx
Outdated
Show resolved
Hide resolved
@@ -81,7 +89,7 @@ export function LegacyMapRedirectHandler({ | |||
const additionalAnalysisConfig = { | |||
...baseAdditionalAnalysisConfig, | |||
...descriptorConfig, | |||
}; | |||
} as AdditionalAnalysisConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using as
, we should declare the type of the variable:
const additionalAnalysisConfig: AdditionalAnalysisConfig = {
This will require declare the components that make up this value (baseAdditionalAnalysisConfig
, and descriptorConfig
). I think this will reveal a subtle type conflict.
Generally, we should avoid as
if possible, and document why we are using it if we need it. This isn't something we've really enforced before.
...ges/sites/genomics-site/webapp/wdkCustomization/js/client/components/Downloads/Downloads.tsx
Outdated
Show resolved
Hide resolved
...ite/webapp/wdkCustomization/js/client/components/genomeSummaryView/EmptyChromosomeFilter.tsx
Outdated
Show resolved
Hide resolved
This looks great. Let's plan to merge after the 68b release. I will set a reminder to approve. |
Let's get this up-to-date w/ the latest from |
This PR upgrades our monorepo packages to React 18, largely by following this React guide.